-
Notifications
You must be signed in to change notification settings - Fork 16
Bypass download if CLI version matches #245
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This is a bit faster than waiting for a 304 but more importantly it works if the user customizes the download source to be a place that does not have the same etag support that Coder does. The test bypasses Windows since we would need to create an executable there for the test. We could split out the exec and the parsing although I am not sure it is worth the noise. The part most likely to break between platforms is probably the exec, not the parsing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change looks fine to me, if we wanted to be a bit more future-proof we would want to do a semver comparison of the versions and ensure that the versions are equivalent instead of just checking the raw string.
src/main/kotlin/com/coder/gateway/views/steps/CoderWorkspacesStepView.kt
Outdated
Show resolved
Hide resolved
9dd62c9
to
a2956e9
Compare
null | ProcessInitException | ||
"""echo '{"foo": true, "baz": 1}'""" | InvalidVersionException | ||
"""echo '{"version: '""" | JsonSyntaxException | ||
"exit 0" | InvalidVersionException | ||
"exit 1" | InvalidExitValueException |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍
This is a bit faster than waiting for a 304 but more importantly it works if the user customizes the download source to be a place that does not have the same etag support that Coder does.
The test bypasses Windows since we would need to create an executable there for the test. We could split out the exec and the parsing although I am not sure it is worth the noise. The part most likely to break between platforms is probably the exec, not the parsing?
We do indirectly test the version parsing on Windows with the test that goes against dev.coder.com.